-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfix - datasource controller was dropping secureJsonData #1488
Conversation
This addresses a bug introduced in the changeover to using the openapi Grafana library where the secureJsonData field was being dropped. To address this it's necessary to use the `UpdateDataSourceCommand` struct to ensure we're also not dropping the version if it's present. This is a bit counter-intuitive to use the `Update` struct here and perhaps worth looking at another solution at some point.
|
Currently awaiting approval from my employer to sign CLA. |
Thanks a lot for this @mdelaney . |
To me, this looks like it works. Thanks allot @mdelaney for your help on this one. |
@smuda can you verify that this fix is working for you? |
Sure, I'll test that tonight |
I'm hoping so. Will update soon as I know more. 🙂 |
I'm a bit confused. I span up the kind cluster and adding a grafanadatasource with appropriate secureJsonData, testing the datasource in grafana and then checks the http message in the "thanos" pod and it misses the Authorization header:
I verified in another kind cluster that (roughly) the same settings created a HTTP post with the Authorization header with an older grafana-operator (v5.6.2):
I might have done something wrong in the verification but it seems this fix is insufficient. I'm going to create a PR from the tag of 5.8.0 with the files to make sure my test work correctly. Files to be added under hack/kind/resources/default (and included in kustomization.yaml)deployment-thanos.yaml: apiVersion: apps/v1
kind: Deployment
metadata:
name: thanos
labels:
app: thanos
spec:
selector:
matchLabels:
app: thanos
template:
metadata:
labels:
app: thanos
spec:
terminationGracePeriodSeconds: 3
containers:
- name: netcat
image: alpine
command:
- sh
- -c
- |
set -eu
echo "Starting pod"
while true; do echo -e 'HTTP/1.1 200 OK\n\n{"asdf":"date"}' | nc -l -p 8080; done
ports:
- containerPort: 8080
name: http
protocol: TCP
---
apiVersion: v1
kind: Service
metadata:
name: thanos
spec:
selector:
app: thanos
ports:
- port: 8080
name: http
protocol: TCP
targetPort: 8080 grafana-datasource-thanos.yaml apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDatasource
metadata:
name: thanos
spec:
datasource:
access: proxy
basicAuth: false
editable: true
isDefault: true
jsonData:
httpHeaderName1: Authorization
timeInterval: 5s
tlsSkipVerify: true
name: Thanos
orgId: 1
secureJsonData:
httpHeaderValue1: Bearer ${token}
type: prometheus
url: http://thanos.default.svc:8080
instanceSelector:
matchLabels:
dashboards: grafana
valuesFrom:
- targetPath: 'secureJsonData.httpHeaderValue1'
valueFrom:
secretKeyRef:
name: grafana-instance-sa-token
key: token secret.yaml apiVersion: v1
kind: Secret
metadata:
name: grafana-instance-sa-token
annotations:
kubernetes.io/service-account.name: grafana-instance-sa
type: kubernetes.io/service-account-token serviceaccount.yaml apiVersion: v1
kind: ServiceAccount
metadata:
name: grafana-instance-sa
secrets:
- name: grafana-instance-sa-token |
I went back to the 5.8.0 release commit, created a branch and added the test files, verifying that they work correctly. Then I rebased on If we could merge that PR and rebase this PR on top, perhaps it would be easer to develop this fix? |
The fix seems to work for me: But I have only verified that the authorization header shows up in the UI, not that it is actually sent when fetching from the datasource. @smuda so th is fix is not working for you? |
@pb82 I'm not saying that I haven't messed anything up during testing, but yes, I do not see the header in the call to my emulator which I did with 5.8.0. But if it works for you, it's likely I have messed up. Here are some example calls I'm seeing with netcat:
|
@smuda I see the token in your test: I would suggest to double-check if you're using a build from this branch. |
:face_palm:
I'm really sorry for making such a mess. |
@smuda No worries, it happens to all of us :) I have some concerns about this implementation as |
Stuff that happens. Thanks allot for the verification and writing the test @smuda . |
@weisdd we don't support orgID at all today so I don't think it's a problem. But let's talk about during today's meeting. |
My concern was that We should be good to merge the PR once @mdelaney signs CLA. Nice work, btw! :) |
Hi all, Unfortunately I haven’t heard back around the CLA and unfortunately can’t provide a good idea of when it will be reviewed. I very much understand if you decide to fix this outside of this pull request. Apologies for the delay on this |
Hi @mdelaney , thanks for the update. Such is life some times I will grab the code and create a separate PR for this so we can solve this issue, but will of course point to this one and credit you for the contribution. Thanks once again for solving the issue. |
This addresses a bug introduced in the changeover to using the openapi Grafana library where the secureJsonData field was being dropped.
To address this it's necessary to use the
UpdateDataSourceCommand
struct to ensure we're also not dropping the version if it's present.This is a bit counter-intuitive to use the
Update
struct here and perhaps worth looking at another solution at some point.fixes #1482 and #1485